Skip to content

Fix webhook SSRF protections - Security Hardening #23568

Open
stonezdj wants to merge 6 commits into
goharbor:mainfrom
stonezdj:26jun22_avoid_csrf
Open

Fix webhook SSRF protections - Security Hardening #23568
stonezdj wants to merge 6 commits into
goharbor:mainfrom
stonezdj:26jun22_avoid_csrf

Conversation

@stonezdj

Copy link
Copy Markdown
Contributor

Thank you for contributing to Harbor!

Comprehensive Summary of your change

Issue being fixed

Fixes #(issue)

Please indicate you've done the following:

  • Well Written Title and Summary of the PR
  • Label the PR as needed. "release-note/ignore-for-release, release-note/new-feature, release-note/update, release-note/enhancement, release-note/community, release-note/breaking-change, release-note/docs, release-note/infra, release-note/deprecation"
  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Made sure tests are passing and test coverage is added if needed.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed in website repository.

@stonezdj
stonezdj requested a review from a team as a code owner July 16, 2026 03:27
@stonezdj
stonezdj force-pushed the 26jun22_avoid_csrf branch from 483222e to 9803eef Compare July 16, 2026 03:28
@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 73.86760% with 75 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.39%. Comparing base (5713e11) to head (fc41e45).

Files with missing lines Patch % Lines
src/lib/outbound_url.go 70.10% 37 Missing and 18 partials ⚠️
...rc/jobservice/job/impl/notification/http_helper.go 79.10% 10 Missing and 4 partials ⚠️
src/jobservice/job/impl/notification/slack_job.go 76.92% 2 Missing and 1 partial ⚠️
...rc/jobservice/job/impl/notification/webhook_job.go 83.33% 2 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #23568      +/-   ##
==========================================
+ Coverage   66.37%   66.39%   +0.01%     
==========================================
  Files        1073     1074       +1     
  Lines      117750   118026     +276     
  Branches     2965     2965              
==========================================
+ Hits        78161    78365     +204     
- Misses      35286    35335      +49     
- Partials     4303     4326      +23     
Flag Coverage Δ
unittests 66.39% <73.86%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/server/v2.0/handler/webhook.go 47.54% <100.00%> (+0.72%) ⬆️
src/jobservice/job/impl/notification/slack_job.go 71.57% <76.92%> (-0.52%) ⬇️
...rc/jobservice/job/impl/notification/webhook_job.go 78.57% <83.33%> (+0.31%) ⬆️
...rc/jobservice/job/impl/notification/http_helper.go 74.41% <79.10%> (+9.20%) ⬆️
src/lib/outbound_url.go 70.10% <70.10%> (ø)

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@stonezdj stonezdj added the release-note/update Update or Fix label Jul 16, 2026
@stonezdj
stonezdj force-pushed the 26jun22_avoid_csrf branch 2 times, most recently from cdbc378 to 04077a3 Compare July 16, 2026 06:53
Comment thread src/lib/outbound_url.go

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Hardens webhook and Slack notification delivery against SSRF.

Changes:

  • Validates targets and resolved IP addresses as public.
  • Uses a restricted transport, blocks redirects, and sanitizes Host headers.
  • Adds SSRF-focused tests.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/lib/outbound_url.go Adds URL, IP, DNS, and dialing validation.
src/lib/outbound_url_test.go Tests outbound URL protections.
src/server/v2.0/handler/webhook.go Validates webhook targets during configuration.
src/server/v2.0/handler/webhook_test.go Tests private-target rejection.
src/jobservice/job/impl/notification/http_helper.go Adds restricted notification transports.
src/jobservice/job/impl/notification/webhook_job.go Validates webhook destinations at execution.
src/jobservice/job/impl/notification/webhook_job_test.go Tests webhook execution protections.
src/jobservice/job/impl/notification/slack_job.go Validates Slack destinations at execution.
src/jobservice/job/impl/notification/slack_job_test.go Tests Slack target rejection.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/jobservice/job/impl/notification/webhook_job.go Outdated
Comment thread src/lib/outbound_url.go Outdated
Comment thread src/jobservice/job/impl/notification/http_helper.go Outdated
Comment thread src/lib/outbound_url.go Outdated
@stonezdj
stonezdj force-pushed the 26jun22_avoid_csrf branch 3 times, most recently from c9ef0c1 to 0efc2ac Compare July 17, 2026 08:15
Copilot AI review requested due to automatic review settings July 23, 2026 05:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Comment thread src/jobservice/job/impl/notification/http_helper.go
func (wj *WebhookJob) execute(ctx job.Context, params map[string]any) error {
payload := params["payload"].(string)
address := params["address"].(string)
validatedAddress, err := lib.ValidatePublicHTTPURL(ctx.SystemContext(), address, true)
func (sj *SlackJob) execute(ctx job.Context, params map[string]any) error {
payload := params["payload"].(string)
address := params["address"].(string)
validatedAddress, err := lib.ValidatePublicHTTPURL(ctx.SystemContext(), address, true)
Copilot AI review requested due to automatic review settings July 23, 2026 06:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (2)

src/lib/outbound_url.go:218

  • The blocklist still accepts non-public IPv6 destinations such as the RFC 8215 local-use translation prefix (64:ff9b:1::/48) and deprecated site-local addresses (fec0::/10): both pass IsGlobalUnicast and IsPrivate. On networks routing either range, webhook targets can still reach internal services, contrary to this validator's contract. Add these ranges and corresponding rejection cases.
		"fc00::/7",      // Unique-Local / ULA (RFC 4193)

src/server/v2.0/handler/webhook_test.go:137

  • Use “should get” for grammatical agreement.
		// private target should got 400

Comment thread src/lib/outbound_url_test.go Outdated
}

// Resolve and validate target host, pinning the IP address to prevent DNS-rebinding SSRF
dialAddr, err := lib.PublicDialAddress(req.Context(), host, port)
TLSHandshakeTimeout: 10 * time.Second,
ExpectContinueTimeout: 1 * time.Second,
}
s.transports[host] = tr
require.NoError(t, err)
req = req.WithContext(context.WithValue(req.Context(), useProxyKey, true))

resp, err := rt.RoundTrip(req)
Comment thread src/server/v2.0/handler/webhook_test.go Outdated
Copilot AI review requested due to automatic review settings July 23, 2026 06:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (6)

src/lib/outbound_url.go:215

  • The denylist omits 64:ff9b:1::/48, the RFC 8215 local-use IPv4/IPv6 translation prefix. Go treats addresses in this range as global unicast, so a translator-enabled network can route an accepted literal through to a non-public IPv4 destination. Block this prefix to preserve the public-only guarantee.
		"100::/64",      // Discard-only address block (RFC 6666)

src/jobservice/job/impl/notification/http_helper.go:143

  • The proxied path still pins only the first DNS result. Because the URL is then rewritten to that IP, neither the proxy nor net.Dialer can fall back to the remaining validated A/AAAA records, so a healthy multi-address webhook fails whenever the first address is unreachable. Preserve the validated address list and retry the request against subsequent addresses under the same context.
	dialAddr, err := lib.PublicDialAddress(req.Context(), host, port)

src/jobservice/job/impl/notification/http_helper.go:159

  • This cache grows permanently for every user-controlled webhook hostname; transports are never removed even after their idle connections expire. A long-lived jobservice processing changing targets can therefore accumulate unbounded transports. Use a bounded/evicting cache or redesign the pinned dialing path so one transport can be shared.
	s.transportsMu.Lock()
	if s.transports == nil {
		s.transports = make(map[string]*http.Transport)
	}
	tr, exists := s.transports[host]
	if !exists {
		tr = &http.Transport{

src/jobservice/job/impl/notification/http_helper_test.go:78

  • This unit test performs a live request to example.com, making its duration and result dependent on external DNS, routing, TLS, and proxy configuration. The error-string fallback still permits long dial timeouts and misses unrelated environment failures. Inject a resolver/dialer or round tripper and verify the rewritten destination without external network access.
		resp, err := rt.RoundTrip(req)

src/server/v2.0/handler/webhook_test.go:101

  • Use “should get” rather than “should got.”
		// private target should get 400

src/server/v2.0/handler/webhook_test.go:137

  • Use “should get” rather than “should got.”
		// private target should get 400

Copilot AI review requested due to automatic review settings July 23, 2026 06:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (3)

src/jobservice/job/impl/notification/http_helper.go:143

  • The proxied path still pins only the first validated A/AAAA result. Because the request URL is then rewritten to that single IP, http.Transport cannot fall back when it is unreachable, so dual-stack and DNS-failover targets can fail through a proxy even though another validated address is healthy. Preserve the validated address list and retry proxied delivery across it, as PublicDialContext now does for direct delivery.
	// Resolve and validate target host, pinning the IP address to prevent DNS-rebinding SSRF
	dialAddr, err := lib.PublicDialAddress(req.Context(), host, port)

src/jobservice/job/impl/notification/http_helper.go:158

  • This cache adds one http.Transport for every webhook hostname and never removes entries. Since target hostnames are user-configurable and jobservice is long-lived, the map and per-transport connection-pool state can grow without bound. Use a bounded/expiring cache or a shared transport design, and close idle connections when evicting entries.
	s.transportsMu.Lock()
	if s.transports == nil {
		s.transports = make(map[string]*http.Transport)
	}
	tr, exists := s.transports[host]
	if !exists {

src/jobservice/job/impl/notification/http_helper_test.go:86

  • This unit test performs a live DNS lookup and request to example.com, then accepts only a hand-picked set of failures. Restricted, proxied, or TLS-intercepted CI can return other errors such as EOF or x509 failures, making the suite nondeterministic; a successful request also does not prove the pinned destination or SNI. Replace the external request with injectable fake resolution/dial/proxy behavior and assert those values directly.
		resp, err := rt.RoundTrip(req)
		if err == nil {
			defer resp.Body.Close()
			// Status can be 200 or 404 or any other HTTP status since it reached example.com
			assert.True(t, resp.StatusCode > 0)
		} else {
			// If network is not reachable (e.g. offline builder), we allow dial/connect errors,
			// but we shouldn't get validation errors.
			assert.True(t, strings.Contains(err.Error(), "dial") || strings.Contains(err.Error(), "connect") || strings.Contains(err.Error(), "lookup") || strings.Contains(err.Error(), "no such host") || strings.Contains(err.Error(), "timeout"))

Comment on lines +164 to +166
TLSClientConfig: &tls.Config{
ServerName: host,
InsecureSkipVerify: s.insecure,
Comment thread src/lib/outbound_url.go
Copilot AI review requested due to automatic review settings July 23, 2026 07:03
@stonezdj
stonezdj force-pushed the 26jun22_avoid_csrf branch from 321b1f5 to ab0f902 Compare July 23, 2026 07:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

src/jobservice/job/impl/notification/http_helper.go:143

  • The proxy path still pins only the first DNS result via PublicDialAddress. If that address is unreachable from the proxy (commonly the first IPv6 result), the request fails without trying the other already-validated records, reintroducing the multi-address availability problem for every proxied notification. Preserve the validated address list and retry safely across it.
	dialAddr, err := lib.PublicDialAddress(req.Context(), host, port)

src/jobservice/job/impl/notification/http_helper.go:173

  • This cache grows permanently for every distinct webhook hostname, and neither entries nor their idle connection pools are ever evicted or closed. Since notification targets are user-configurable, repeated jobs targeting unique public hosts can cause unbounded memory and connection-resource retention in the long-lived jobservice. Use a bounded/evicting cache that closes idle connections, or avoid retaining one transport per hostname.
		s.transports[host] = tr

src/jobservice/job/impl/notification/http_helper_test.go:78

  • This test performs real DNS, proxy, TCP, and TLS operations against example.com, so its result depends on the builder network and trust configuration; for example, TLS interception produces an x509 error that the allowlist rejects. It also does not prove a proxy was used when no proxy environment variable is set. Replace it with a deterministic local fake proxy and controlled target resolution.
		resp, err := rt.RoundTrip(req)

Comment thread src/lib/outbound_url.go
if err := ctx.Err(); err != nil {
return nil, err
}
conn, err := publicDialer.DialContext(ctx, network, dialAddress)
@stonezdj
stonezdj force-pushed the 26jun22_avoid_csrf branch from ab0f902 to dd06e66 Compare July 23, 2026 07:14
stonezdj and others added 6 commits July 23, 2026 16:31
  - Add function ValidatePublicHTTPURL and PublicDialContext to validate the address is public address
  - Verify the address before sending webhook and slack http request

Signed-off-by: stonezdj <stone.zhang@broadcom.com>
Signed-off-by: stonezdj <stone.zhang@broadcom.com>
Signed-off-by: stonezdj <stone.zhang@broadcom.com>
Signed-off-by: stonezdj <stone.zhang@broadcom.com>
Signed-off-by: stonezdj <stone.zhang@broadcom.com>
Signed-off-by: stonezdj <stone.zhang@broadcom.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants